AppSideNav: fix scrolling issue and being able to focus hidden links issue
#2869
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
shleewhite
commented
May 12, 2025
shleewhite
changed the title
AppSideNav: fix scrolling issue, AppSideNav: fix scrolling issue and being able to focus hidden links issue
📦 RC Packages PublishedLatest commit: 345c550 |
shleewhite
commented
May 13, 2025
|
|
||
| More specifically: | ||
|
|
||
| - `minimized → maximized` transition: the content appears with a fade-in effect, when the width animation is already completed (the width is maximized) | ||
| - `maximized → minimized` transition: the content disappears at once with no transition, before the width animation starts | ||
|
|
||
| Another option is to use the **`isMinimized` parameter**, which is useful in those cases where the content is so custom/specialized that it can’t just be faded in/out but needs to have a different kind of transition (eg. remain visible but change layout or respond to the width of the container). This value is passed down by the `<:header/body/footer>` named blocks as parameters, and can be used to build custom logic on the consumers’ side. |
There was a problem hiding this comment.
The component doesn't return named blocks or the isMinimized state to the consumer, so this is not true.
website/docs/components/app-side-nav/partials/code/how-to-use.md
Outdated
Show resolved
Hide resolved
didoo
reviewed
May 14, 2025
1 task
Co-authored-by: Kristin Bradley <[email protected]>
…ide-nav--hide-when-minimized
shleewhite
force-pushed
the
hds-4827/app-side-nav-overflow-bug
branch
from
1a2188c to
ffb7563
Compare
didoo
reviewed
May 14, 2025
There was a problem hiding this comment.
@shleewhite left a bunch of comments/questions, but overall the PR is so much better and clean with your latest changes! 🙌
showcase/tests/integration/components/hds/app-side-nav/index-test.js
Outdated
Show resolved
Hide resolved
showcase/tests/integration/components/hds/app-side-nav/index-test.js
Outdated
Show resolved
Hide resolved
showcase/tests/integration/components/hds/app-side-nav/index-test.js
Outdated
Show resolved
Hide resolved
…est.js Co-authored-by: Cristiano Rastelli <[email protected]>
didoo
approved these changes
May 15, 2025
shleewhite
added
release-candidate
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
📌 Summary
If merged, this PR would do some cleanup of the AppSideNav component
There are also updates to tests to check for the correct behavior (+ 1 bonus fix for a Modal test):
overflow: hiddenis set appropriatelybodyelement in tests🧪 Tested in Atlas:
🔗 External links
Jira ticket: HDS-4827
Jira ticket: HDS-4858
👀 Component checklist
💬 Please consider using conventional comments when reviewing this PR.